ROX-32966: Add disabling label for the konflux retests#81
ROX-32966: Add disabling label for the konflux retests#81
Conversation
| github.event.check_run.conclusion == 'failure' && | ||
| startsWith(github.event.check_run.name, 'Red Hat Konflux') | ||
| startsWith(github.event.check_run.name, 'Red Hat Konflux') && | ||
| !contains(github.event.check_run.pull_requests[0].labels.*.name, 'disable-konflux-auto-retest') |
There was a problem hiding this comment.
we know that there is a least one PR associated with the check (see check above). My understanding is that the Konflux check might have multiple PRs only when the same commit is used in those PRs. I assume it's quite rare and unique scenario so I believe [0] is correct instead of * (we don't want to disable if at least one PR has label)
There was a problem hiding this comment.
Above you have github.event.check_run.pull_requests[0] != null. How does it evaluate when there are no pull requests? MintMaker does this when we configure automerge: branch, i.e. in https://github.com/stackrox/operator-index/blob/master/.github/renovate.json5#L51
Does it evaluate as "false" or does it crash GHA with sort-of a NullPointerException or is the result of github.event.check_run.pull_requests[0] equal to undefined and the expression github.event.check_run.pull_requests[0] != null weirdly evaluates to true?
Wouldn't it be more robust to check length(github.event.check_run.pull_requests) == 1 if such thing is possible?
There was a problem hiding this comment.
My bad, I didn't reflect the last changes. So the latest approach (which you have already tested in the https://github.com/stackrox/test-gh-actions/pull/240) is to pull actual label value directly from GH API instead of relying on event data. Your previous testing proofed that event data (github.event.check_run.pull_requests[0].labels.*.name) might be outdated. So I've updated the PR and also changed null check to github.event.check_run.pull_requests.length > 0
msugakov
left a comment
There was a problem hiding this comment.
Regarding
No description provided.
Is there any way you can test this before merging to see if it really works?
I planned to test it with this PR, but looks like konflux is not starting for that repo ATM. https://github.com/stackrox/test-gh-actions/pull/236. Looks like this testing Konflux app was deleted. I will add a new one |
msugakov
left a comment
There was a problem hiding this comment.
Given now it passes testing, I've a couple of things to clarify.
| github.event.check_run.conclusion == 'failure' && | ||
| startsWith(github.event.check_run.name, 'Red Hat Konflux') | ||
| startsWith(github.event.check_run.name, 'Red Hat Konflux') && | ||
| !contains(github.event.check_run.pull_requests[0].labels.*.name, 'disable-konflux-auto-retest') |
There was a problem hiding this comment.
Above you have github.event.check_run.pull_requests[0] != null. How does it evaluate when there are no pull requests? MintMaker does this when we configure automerge: branch, i.e. in https://github.com/stackrox/operator-index/blob/master/.github/renovate.json5#L51
Does it evaluate as "false" or does it crash GHA with sort-of a NullPointerException or is the result of github.event.check_run.pull_requests[0] equal to undefined and the expression github.event.check_run.pull_requests[0] != null weirdly evaluates to true?
Wouldn't it be more robust to check length(github.event.check_run.pull_requests) == 1 if such thing is possible?
| if echo "$CURRENT_LABELS" | grep -q "^disable-konflux-auto-retest$"; then | ||
| echo "Skipping retest: disable-konflux-auto-retest label is present" | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
It seems like a new implementation that you landed 20 hours ago (according to GitHub). When I look at last comments/updates in https://github.com/stackrox/test-gh-actions/pull/240, they are from 2 days ago (according to GitHub).
Would you like to test the change?
| CURRENT_LABELS=$(gh api "repos/${{ github.repository }}/pulls/$PR_NUMBER" --jq '.labels[].name') | ||
| if echo "$CURRENT_LABELS" | grep -q "^disable-konflux-auto-retest$"; then |
There was a problem hiding this comment.
- Why
gh api, can't something like this work?
$ gh pr view --json labels --jq '.labels[].name' https://github.com/stackrox/stackrox/pull/18943-
Please use quoting around command substitution:
VAR=$(command)- wrong
VAR="$(command)"- better -
There's probably no need to define a variable to use it once.
Instead of
CURRENT_LABELS="$(gh ...)"
if echo "$CURRENT_LABELS" | grep ...; thenyou can write
if gh ... | grep ...; thenThere was a problem hiding this comment.
- The
gh apiis used in other lines so for consistency it's better to usegh apias well. But you are right thatgh pr viewcan also be used. - Added quoting. My bad, I always forget about it
- I would prefer to have two lines. Yes, it's slightly verbose but it's more readable
Co-authored-by: Misha Sugakov <537715+msugakov@users.noreply.github.com>
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| RETEST_COMMAND: ${{ inputs.retest_command }} | ||
| run: | |
There was a problem hiding this comment.
Please add set -euo pipefail mantra here too.
Testing: https://github.com/stackrox/test-gh-actions/pull/240